-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support TestMessageStackFrame API #14154
Conversation
Also properly convert TestState back from TestStateChangeDTO. fixes eclipse-theia#14111 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
afa8bc0
to
8bfe22d
Compare
export namespace TestFailureDTO { | ||
export function is(ref: unknown): ref is TestFailureDTO { | ||
return isObject<TestFailureDTO>(ref) | ||
&& ref.state === (TestExecutionState.Failed || TestExecutionState.Errored); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work: (TestExecutionState.Failed || TestExecutionState.Errored)
will always evaluate to TestExecutionState.Failed
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thanks for catching. Fixed in 2nd commit
this.testStates.set(item, change); | ||
stateEvents.push({ test: item, oldState: oldState, newState: change }); | ||
// convert back changes (for example, convert back location from DTO) | ||
const convertedState = convertTestState(change); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this conversion now? Aren't the types compatible anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the Position for example are not the same structure (line vs lineNumber for example). There is also the issue of 0-based or 1-based indexes on Position and Range.
TestMessage location
is not used for example, but it still needs conversion to be exact for usage in the future. Same for properties of TestMessageStackFrame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but Position
is an interface that we define ourselves in this PR, so we can make it compatible. As for the zero/one based index conversion, aren't we doing the on the ext side in general? If we have an explicit TestStateChangeDTO
we can design it in a way that is compatible with the browser side structure, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be done, while adding some new converters on ext side to the language server types.
I pushed a new version that removes this conversion.
frameElement.append(' from '); | ||
const link = this.node.ownerDocument.createElement('a'); | ||
link.textContent = `${this.labelProvider.getName(uri)}`; | ||
link.title = `${uri}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using title
here instead of href
prevents the UI from rendering the anchor tag as a proper link. No underlining and the cursor does not change to a pointing hand when hovering. Any particular reason for doing it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no particular reason. That's fixed.
if (stackFrame.uri) { | ||
|
||
const uri = stackFrame.uri; | ||
frameElement.append(' from '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I18N, also, it would be nice if we could make this look a bit like a stack trace, i.e. similar to
Uncaught (in promise) TypeError: Cannot destructure property 'startLineNumber' of 'range' as it is undefined.
at MonacoOutlineContribution.asRange (monaco-outline-contribution.ts:260:17)
at MonacoOutlineContribution.getFullRange (monaco-outline-contribution.ts:281:21)
at MonacoOutlineContribution.parentContains (monaco-outline-contribution.ts:245:62)
at MonacoOutlineContribution.applySelection (monaco-outline-contribution.ts:227:26)
at MonacoOutlineContribution.createRoots (monaco-outline-contribution.ts:156:14)
at MonacoOutlineContribution.updateOutline (monaco-outline-contribution.ts:113:43)
at monaco-outline-contribution.ts:97:92
at event.ts:176:42
at CallbackList.invoke (event.ts:184:26)
at Emitter.fire (event.ts:327:36)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will squash and update the changelog once the PR is accepted. |
- remove conversion on test package - adapt DTOs to allow 1-to-1 mapping on browser side - add required convertes on plugin-ext side
@rschnekenbu there now seems to be a conversion problem: when I click on the line |
Indeed, the location which is used to navigate is based on the 0-based one, but the tool obviously displays the Monaco coordinates. So I will fix the location shown in the Test Results to be the Monaco location. |
What it does
Adds 'stacktraces: TestMessageStackFrame' to TestMessage. This optional property allows extensions to describe the context in which an error could occur when test is failing.
Here is a snapshot as an example on vscode. They embed small monaco editors to display the line and a bit around to display the stack trace when the test is failing.
Note that our current implementation of Test is lighter than the vs code one. So a more simple approach was given, with only the label of the StackTrace and a hyperlink to navigate to the file located at the uri given by the stack frame, as well as the cursor position.
Fixes #14111
Contributed on behalf of STMicroelectronics
How to test
The attached extension is based on test-sample-provider from vscode-extension-samples:
Follow-ups
None known
Review checklist
Reminder for reviewers